-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CHORE] Update Frequency to v0.9.30 #744
Conversation
Per my Slack message, I recommend test running all 3 CI workflows before merging #744 to main, so there are now surprises later. Let me know if you need my help with it when you are ready to merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a nitpick, but it would help with reviewing if you could summarize the changes involved in the upgrade, so I can know what to expect and review more efficiently. There was a lot of renaming involved, it looks like, as well as some parameter additions and reordering in cumulus.
@@ -339,6 +339,15 @@ pub fn run() -> Result<()> { | |||
)?; | |||
cmd.run(partials.client) | |||
}), | |||
#[cfg(not(feature = "runtime-benchmarks"))] | |||
frame_benchmarking_cli::BenchmarkCmd::Storage(_) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this required for the upgrade to v0.9.30?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was a possible panic when someone was running storage benchmarks without the flags, but since template node has updated it so has others
@@ -308,7 +308,7 @@ where | |||
let prometheus_registry = parachain_config.prometheus_registry().cloned(); | |||
let transaction_pool = params.transaction_pool.clone(); | |||
let import_queue = cumulus_client_service::SharedImportQueue::new(params.import_queue); | |||
let (network, system_rpc_tx, start_network) = | |||
let (network, system_rpc_tx, tx_handler_controller, start_network) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: If these parameter changes are part of the update it would be nice to note it. I would prefer not to assume so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes these are changes in the underlying apis
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This update is interesting its a step towards separating collator with embedded relay paritytech/cumulus#1585
@@ -88,7 +88,7 @@ pub mod pallet { | |||
#[pallet::config] | |||
pub trait Config: frame_system::Config { | |||
/// The overarching event type. | |||
type Event: From<Event<Self>> + IsType<<Self as frame_system::Config>::Event>; | |||
type RuntimeEvent: From<Event<Self>> + IsType<<Self as frame_system::Config>::RuntimeEvent>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this also renamed in the new substrate version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codecov Report
@@ Coverage Diff @@
## main paritytech/substrate#744 +/- ##
=======================================
Coverage 79.58% 79.58%
=======================================
Files 19 19
Lines 823 823
=======================================
Hits 655 655
Misses 168 168
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
…requency into chore/upgrade_0.9.32
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some questions but overall looks good.
In Rust compiler We Trust
integration-tests/package.json
Outdated
@@ -13,7 +13,7 @@ | |||
"author": "", | |||
"license": "Apache-2.0", | |||
"dependencies": { | |||
"@frequency-chain/api-augment": "v0.9.29", | |||
"@frequency-chain/api-augment": "v0.9.30", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how is this going to affect metadata for other libraries?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think exacting the version might complicate this we should do "^v0.9.29"
for now given I am not sure at what stage these tests are run but for sure npm wont have this version till we release
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only one thing that squeaked in that must change, but a few other questions and I think we want to run benchmarks before merging.
integration-tests/package.json
Outdated
"@frequency-chain/api-augment": "^v0.9.29", | ||
"@polkadot/api": "^9.6.2", | ||
"@polkadot/types": "^9.6.2", | ||
"@polkadot/util": "^8.7.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a reversion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this to pin the version on which integration tests run fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see yes something updated since this PR let me keep whats in main
@@ -570,7 +569,7 @@ where | |||
"Failed to create parachain inherent", | |||
) | |||
})?; | |||
Ok((time, slot, parachain_inherent)) | |||
Ok((slot, timestamp, parachain_inherent)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These order swaps worry me. It feels like a mistake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this is change observed in the way they have modified starting node due to upcoming changes
Here is a boilerplate PR https://github.com/substrate-developer-hub/substrate-parachain-template/pull/133
Its a change across substrate
@@ -91,6 +91,7 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> { | |||
// Standard Error: 139_000 | |||
.saturating_add(Weight::from_ref_time(4_711_000 as u64).saturating_mul(s as u64)) | |||
.saturating_add(T::DbWeight::get().reads(1 as u64)) | |||
.saturating_add(T::DbWeight::get().writes(2 as u64)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where did these come from?
Also we should likely run benchmarks before merging right? Lots of weights could/should have changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Older weights than Main , need to run benchmarks before merge to update these
@@ -122,6 +122,7 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> { | |||
// Standard Error: 6_000 | |||
.saturating_add(Weight::from_ref_time(1_193_000 as u64).saturating_mul(s as u64)) | |||
.saturating_add(T::DbWeight::get().reads(2 as u64)) | |||
.saturating_add(T::DbWeight::get().writes(5 as u64)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another weights update I didn't expect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these are just older weights, I need to run the benchmarks on this PR
@@ -157,7 +164,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion { | |||
spec_name: create_runtime_str!("frequency"), | |||
impl_name: create_runtime_str!("frequency"), | |||
authoring_version: 1, | |||
spec_version: 3, | |||
spec_version: 4, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙇
@@ -13,7 +13,7 @@ | |||
"author": "", | |||
"license": "Apache-2.0", | |||
"dependencies": { | |||
"@frequency-chain/api-augment": "v0.9.29", | |||
"@frequency-chain/api-augment": "^v0.9.29", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is mostly done as .9.30 wont be available until release, so pinning on last stable release
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to make it so that for CI this builds and uses the "local" version so they stay in sync for ci. On my need a break list ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved (benchmarks are running)
Goal
The goal of this PR is to upgrade substrate to polkadot release .9.30
Closes paritytech/substrate#704
Notes:
PR for Substrate upgrade to 0.9.30
Release Notes
Major updates from 0.9.29 to 0.9.30:
Only breaking changes: Rename of Enums in Pallet Config
Call
->RuntimeCall
Event
->RuntimeEvent
andOrigin
->RuntimeOrigin
.Here is the PR BREAKING: Rename Call & Event paritytech/substrate#11981
Upgrades from Polkadot v0.9.29 to v0.9.30, version update in dependencies tree
Weights update: V2 is slowly getting released and benign updates carried in this PR. The rest of the way to Weights V2 (Tracking Issue) paritytech/polkadot-sdk#256 and Add storage size component to weights paritytech/substrate#12277
@enddynayn @shannonwells some updates in vesting pallet Vesting pallet - make WithdrawReasons configurable paritytech/substrate#12109
@wilwade some pubsub stuff rpc: Implement
transaction
RPC API paritytech/substrate#12328@wilwade Enable collation via RPC relay chain node paritytech/cumulus#1585
@shannonwells some updates in Democracy Bound uses of
Call
paritytech/substrate#11649Checklist